Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Pino transport to be used as transport #1151

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Nov 21, 2024

See Intercom conversation, test setup, docs change.

The Pino transport could not be used as a transport (that is, as initialised by pino.transport(), but only as a destination, and only in the main thread.

This commit makes the Pino transport communicate with the extension directly, which should work from any worker thread -- the assumption being that the extension has been started by the main thread.

This works around the fact that the client cannot be serialised and sent across threads.

This allows for the Pino transport to be used like other transports, and alongside other transports:

const logger = pino({
  transport: {
    targets: [
      { target: "@appsignal/nodejs/pino", options: { group: "pino" } },
      { target: ... }
    ]
  }
})

The pino/index.js file has been added as a convenience to allow for the target to be referred to as @appsignal/nodejs/pino, which is nicer-looking than the transport's real location inside dist/.

@unflxw unflxw self-assigned this Nov 21, 2024
@unflxw unflxw changed the title Fix Pino transport Allow Pino transport to be used as transport Nov 21, 2024
The Pino transport could not be used as a transport (that is, as
initialised by `pino.transport()`, but only as a destination, and
only in the main thread.

This commit makes the Pino transport communicate with the extension
directly, which should work from any worker thread -- the assumption
being that the extension has been started by the main thread.

This works around the fact that the client cannot be serialised and
sent across threads.

This allows for the Pino transport to be used like other transports,
and _alongside_ other transports:

```js
const logger = pino({
  transport: {
    targets: [
      { target: "@appsignal/nodejs/pino", options: { group: "pino" } },
      { target: ... }
    ]
  }
})
```

The `pino/index.js` file has been added as a convenience to allow for
the target to be referred to as `@appsignal/nodejs/pino`, which is
nicer-looking than the transport's real location inside `dist/`.
@unflxw unflxw force-pushed the pino-transport-as-transport branch from ad72f5a to 11b789d Compare November 21, 2024 14:10
Copy link
Member

@tombruijn tombruijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will review again when the test app works.

src/pino_transport.ts Show resolved Hide resolved
@unflxw unflxw merged commit 6df38ca into main Nov 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants